Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add color interpolation bar on each channel in ColorPicker #46110

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

gongpha
Copy link
Contributor

@gongpha gongpha commented Feb 17, 2021

Like the most ColorPicker from several engines. For easy to visualize available colors in the channel.

Only shown in RGB and HSV mode. ( except Alpha )

ภาพ
ภาพ
Default theme :
ภาพ

Edit: issues in pictures

@RandomShaper
Copy link
Member

A way to switch between the old and the new visualization would be very welcome. Maybe an editor setting, or, even better, a chefk box there at the color picker itself (it's already quite full, though).

@YuriSizov
Copy link
Contributor

What’s up with the spinboxes to the right, though?

@groud
Copy link
Member

groud commented Feb 17, 2021

A way to switch between the old and the new visualization would be very welcome. Maybe an editor setting, or, even better, a chefk box there at the color picker itself (it's already quite full, though).

What would be the point of it? This new version is simply better, and I don't believe we need to keep compatibility here.
This is mainly an editor/tools control node, I don't think it is often used in-game as is.

What’s up with the spinboxes to the right, though?

This issue is likely fixed, there was an issue on master making some LineEdit/SpinBoxes too small. See #46088

@RandomShaper
Copy link
Member

What would be the point of it? This new version is simply better, and I don't believe we need to keep compatibility here.
This is mainly an editor/tools control node, I don't think it is often used in-game as is.

You're right. I was giving an opinion as if the older sliders were displaying (for RGB) black-to-red, black-to-green and black-to-blue. My memory played a trick on me. That's why I thought the old way should be kept as an option.

Of course, the one this PR implements is far superior than the current flat colored sliders. 😃

@KoBeWi
Copy link
Member

KoBeWi commented Feb 17, 2021

HSV mode is bugged:
Qw02vDINBx

  • Changing Hue doesn't affect other sliders (notice how increasing Saturation changes the color of its slider).
  • Hue doesn't update with Saturation/Value changes.
  • Value affects Alpha slider.

@gongpha
Copy link
Contributor Author

gongpha commented Feb 18, 2021

  • Changing Hue doesn't affect other sliders (notice how increasing Saturation changes the color of its slider).

  • Hue doesn't update with Saturation/Value changes.

  • Value affects Alpha slider.

This also happened in the above HSV box, and I don't know about the HSV function. I may be to find a way to fix these bugs with another solution.

@fire
Copy link
Member

fire commented Feb 18, 2021

I think [this] PR should stand alone without these features, but I was wondering if you were also interested in making alternative mappings like: HSY' and other shapes.

We have to provide HSV, but it's not that great. There are better gradient generators that give better results in perceptual space.

image

image

@gongpha
Copy link
Contributor Author

gongpha commented Feb 19, 2021

I think [this] PR should stand alone without these features, but I was wondering if you were also interested in making alternative mappings like: HSY' and other shapes.

We have to provide HSV, but it's not that great. There are better gradient generators that give better results in perceptual space.

If someone will tell, This is enough to have only a gradient box. I honestly to leave them.

I know a bit HSY' color model. This would be okay to implement it when it's commonly used in the engine. BTW, I'm really interested.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took another look and this is actually good (now after #46230 was merged). The new bar_arrow theme icon needs documentation though (you can use doctool to generate blank entry and fill the description).

Also, this is not critical, but I still think it would be nice if the hue bar in HSV was drawn with proper saturation and value. Right now it's not affected by other sliders.

@YuriSizov
Copy link
Contributor

Also, this is not critical, but I still think it would be nice if the hue bar in HSV was drawn with proper saturation and value. Right now it's not affected by other sliders.

Wasn't that fixed in #46230 specifically to make it work here?

@KoBeWi
Copy link
Member

KoBeWi commented Mar 7, 2021

Wasn't that fixed in #46230 specifically to make it work here?

No, it fixed another thing. See here:
Pickqr
The HSV bar is drawn as texture, but changing saturation and value doesn't update its modulation. As I said, it's not critical and we can merge as is right now if it proves too difficult to do (value should be easy, but not sure about saturation).

@gongpha gongpha requested a review from a team as a code owner March 9, 2021 07:59
@gongpha gongpha requested review from KoBeWi and removed request for a team March 9, 2021 08:01
@akien-mga akien-mga merged commit 18bb367 into godotengine:master Mar 9, 2021
@akien-mga
Copy link
Member

Thanks!

@gongpha gongpha deleted the colorbar-in-colorpicker branch March 9, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants